-
Notifications
You must be signed in to change notification settings - Fork 191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Staircased embedded boundaries in the YEE solver #1881
Conversation
amrex::Real H_theta = 0; | ||
amrex::Real mu_r = PhysConst::mu0; | ||
#ifdef AMREX_USE_EB | ||
amrex::Real H_phi = k / (r_sphere * mu_r) * boost::math::sph_bessel(1, k*r) * sin(theta) * cos(k * PhysConst::c * t); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed on Slack:
yes we do not depend on boost because it's a pretty heavy dependency to pull in. But if you only need them on the host-side, you can take the C++17 functions!
https://en.cppreference.com/w/cpp/numeric/special_functions/sph_bessel
We are compiling with C++14 or newer, but we can with no problem require C++17 for selected features already! (We will probably transition to require C++17 anyway during the year.)
So it would be no problem to say e.g. "if you want to use embedded boundaries, use a more recent compiler", absolutely no problem.
amrex::Real H_r = 0; | ||
amrex::Real H_theta = 0; | ||
amrex::Real mu_r = PhysConst::mu0; | ||
amrex::Real H_phi = k / (r_sphere * mu_r) * std::sph_bessel(1, k*r) * sin(theta) * cos(k * PhysConst::c * t); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided to already require C++17 for EB features (instead of pulling in external dependencies).
memo to myself: reflect that in build systems and add checks at compile time asserting __cplusplus
version.
I opened a feature request to Nvidia about future support of spherical bessel functions on device.
As discussed offline with @lgiacome, the automated tests will be slightly modified to consider a cubic conducting boundary, instead of a spherical one. This will make it possible to initialize the fields with the parser, and will also remove the need to compute the spherical Bessel function. Thanks again for this PR @lgiacome ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the tests! This looks very good.
I have a few more comments (see inline comments).
ds = yt.load(filename) | ||
data = ds.covering_grid(level=0, left_edge=ds.domain_left_edge, dims=ds.domain_dimensions) | ||
|
||
tol_err = 1e-5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that this value is probably too high for this test to properly check the simulated fields.
More specifically, if I didn't make any mistake, I think even if Bx_sim
/By_sim
/Bz_sim
were completely wrong (e.g. Bx_sim = By_sim = Bz_sim = 0
), then the err_x
/err_y
/err_z
would be on the order of 1.e-7
or 1.e-6
, i.e. below tol_err
, which means that the test would pass.
One could use a more stringent value for tol_err
.
Or one alternative maybe is to use a relative error like this:
rel_tol_err = 0.1
Bx_sim = data['Bx'].to_ndarray()
rel_err_x = np.sqrt( np.sum(np.square(Bx_sim - Bx_th)) / np.sum(np.square(Bx_th)) )
assert(rel_err_x < rel_tol_err )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! The relative error you are suggesting seems good to me, I will implement it. Thanks a lot for pointing out this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have implemented the relative error. By the way, I am not checking the error on Bx anymore as it is zero everywhere and the relative error would blow up.
Co-authored-by: Remi Lehe <remi.lehe@normalesup.org>
Co-authored-by: Remi Lehe <remi.lehe@normalesup.org>
This pull request introduces 1 alert when merging 35705d0 into 5035351 - view on LGTM.com new alerts:
|
Thanks for incorporating the requested changes! 🚀 |
Adding embedded boundaries with staircasing approximation in the YEE solver.
The changes include:
There is a small issue with the code implemented in PR #1641. When I compile with the Embedded Boundary option and I don't specify any conductor in the input file I get the following error message
amrex::Abort::0::geom_type 1 not supported !!!
while the geometry should be interpreted as
all_regular
.This issue is now fixed by this PR